Greptile Summary

This PR decouples SCIM group state from the Team model by introducing a lightweight ScimGroup record, and wires both SCIM and OIDC group provisioning through a single shared group-mappings.ts service. The architecture change is well-motivated and the implementation addresses every critical issue raised in previous review rounds:

• Role downgrade fixed — applyMappedMemberships uses a ROLE_RANK guard and only ever upgrades an existing role, never overwrites a higher one.
• Stale mappings on rename fixed — both PATCH and PUT re-call getMappingsForGroup after a displayName update so subsequent member operations use the correct mapping.
• Cascading deletes fixed — DELETE no longer touches TeamMember; adopted POST now correctly returns the freshly-updated ScimGroup record.
• Role ambiguity fixed — resolveScimRole is removed in favour of explicit per-group mappings.

Key remaining behavioral limitation: SCIM member remove and full-replace PUT are both additive-only. The code is clear and well-commented about why (no provenance field on TeamMember), but the PR description's test plan still includes "SCIM removes member — verify TeamMember removed" — a test that will never pass. Deprovisioning is deferred until the user's next OIDC login. Operators must be informed that SCIM-sourced access revocation is eventual, not immediate, and the test plan should be corrected to match the implementation's intentional design.

Confidence Score: 3/5

• Safe to merge for structural improvements; operators must understand that SCIM access revocation is eventual (next OIDC login), not immediate.
• All critical correctness bugs from previous reviews are addressed (role downgrades, stale mappings, cascading deletes). The architecture is clean. However, the PR description's test plan contradicts the implementation's intentional design decision to not remove team members via SCIM (by design, no provenance tracking). This gap must be communicated in documentation and the test plan must be corrected before merge. Score is held at 3 rather than 4 because this behavioral limitation — while correctly implemented — contradicts the stated test expectations.
• src/app/api/scim/v2/Groups/[id]/route.ts — test plan documentation correction required before merge

Sequence Diagram

sequenceDiagram
    participant IdP as Identity Provider
    participant SCIM as SCIM Endpoint
    participant GM as group-mappings.ts
    participant DB as PostgreSQL

    IdP->>SCIM: POST /Groups {displayName, externalId, members?}
    SCIM->>DB: findUnique ScimGroup (by displayName)
    alt Group exists
        SCIM->>DB: update ScimGroup (externalId if changed)
        SCIM-->>IdP: 200 OK (updated record)
    else New group
        SCIM->>DB: create ScimGroup
        SCIM-->>IdP: 201 Created (no member provisioning)
    end

    IdP->>SCIM: PATCH /Groups/{id} ops:[add members]
    SCIM->>GM: loadGroupMappings()
    GM->>DB: findUnique SystemSettings.oidcTeamMappings
    GM-->>SCIM: GroupMapping[]
    SCIM->>GM: getMappingsForGroup(allMappings, displayName)
    loop each member userId
        SCIM->>GM: applyMappedMemberships(tx, userId, groupMappings)
        loop each mapping
            GM->>DB: findUnique TeamMember
            alt No existing membership
                GM->>DB: create TeamMember (mapped role)
            else Existing role is lower
                GM->>DB: update TeamMember (upgrade role only)
            end
        end
    end
    SCIM-->>IdP: 200 OK

    IdP->>SCIM: PATCH /Groups/{id} ops:[remove members]
    Note over SCIM: Remove is a no-op (no provenance tracking)<br/>Access revoked only on next OIDC login
    SCIM-->>IdP: 200 OK (membership retained until next OIDC login)

    IdP->>SCIM: PUT /Groups/{id} {displayName, members}
    SCIM->>GM: loadGroupMappings() + getMappingsForGroup()
    Note over SCIM: Additive-only sync — adds present members, never removes
    SCIM-->>IdP: 200 OK

    IdP->>SCIM: DELETE /Groups/{id}
    SCIM->>DB: delete ScimGroup (TeamMembers NOT cascaded)
    SCIM-->>IdP: 204 No Content

_{Last reviewed commit: 5f6d8ef}

@greptile review.

┌──────────────────────┬─────────────────────────────────┬────────────────────────────────┐
│ Finding │ Fix │ Impact │
├──────────────────────┼─────────────────────────────────┼────────────────────────────────┤
│ DELETE cascade wipes │ Removed teamMember.deleteMany │ Prevents mass deprovisioning │
│ all members │ cascade entirely │ when one SCIM group is deleted │
├──────────────────────┼─────────────────────────────────┼────────────────────────────────┤
│ PUT full-sync │ Changed to additive-only using │ Users from other │
│ removes cross-group │ applyMappedMemberships │ groups/OIDC/manual assignment │
│ members │ │ stay intact │
├──────────────────────┼─────────────────────────────────┼────────────────────────────────┤
│ POST returns stale │ Capture and return adopted from │ IdP sees current state, │
│ adopted record │ update │ preventing re-create conflicts │
└──────────────────────┴─────────────────────────────────┴────────────────────────────────┘

@greptile fixed.

PATCH remove cross-group data │ Made remove a no-op with comment; deleted │
│ loss │ removeMappedMemberships │
├────────────────────────────────┼────────────────────────────────────────────────────────┤
│ Stale mapping on rename + │ DisplayName rename now re-resolves groupMappings │
│ member batch │ immediately

@greptile fixed

Finding 1 (PUT stale mappings): Same pattern we fixed in PATCH — re-resolve after rename.
Legitimate.

Finding 2 (role downgrade): Real issue. If "admins" maps to Engineering/ADMIN and "developers"
maps to Engineering/EDITOR, syncing "developers" would overwrite ADMIN→EDITOR. Fix: only
upgrade, never downgrade.